-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix STJ source generator emitting culture-dependent minus signs for negative JsonPropertyOrder #121278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…alues Co-authored-by: stephentoub <[email protected]>
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
|
@copilot, are there any other cases of this in the STJ emitter? Places where something like a number or DateTime or something else that's culture-sensitive may end up rendering with the current culture inappropriately? I'm wondering whether a more encompassing fix might be to instead surround the emitter with setting CultureInfo.CurrentCulture to InvariantCulture (and then setting it back in a finally). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a culture-specific bug in the JSON source generator where negative JsonPropertyOrder values could generate invalid C# code in locales that use non-ASCII minus signs (e.g., Finnish locale uses U+2212 instead of U+002D).
Key changes:
- Modified the source generator to use culture-invariant formatting when emitting property order values
- Added a comprehensive unit test that validates the fix by testing code generation under the Finnish locale
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs | Fixed numeric formatting to use InvariantCulture when generating code for property.Order values |
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs | Added test to verify negative JsonPropertyOrder values generate valid code across different locales |
...tem.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs
Outdated
Show resolved
Hide resolved
I searched through the emitter and found:
However, wrapping the entire
I'll implement that approach - setting |
Co-authored-by: stephentoub <[email protected]>
@copilot, you didn't make the changes. Please wrap the invocation of the emitter with such a try/finally. |
Co-authored-by: stephentoub <[email protected]>
I've implemented the try/finally wrapper approach. The Emit invocation is now wrapped with code that sets |
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn3.11.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn4.0.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn4.0.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn4.0.cs
Outdated
Show resolved
Hide resolved
...tem.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs
Outdated
Show resolved
Hide resolved
…atting, use ThreadCultureChange helper Co-authored-by: stephentoub <[email protected]>
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn3.11.cs
Outdated
Show resolved
Hide resolved
…erator Co-authored-by: stephentoub <[email protected]>
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn3.11.cs
Outdated
Show resolved
Hide resolved
|
/ba-g all failures known / unrelated |
Fix STJ source generator producing non-compilable code on some locales
This PR addresses an issue where the System.Text.Json source generator produces non-compilable code when using
JsonPropertyOrderwith negative values on certain locales (e.g., fi_FI.UTF-8).Changes Made
Technical Details
The issue was in the source generator where numeric values were being directly interpolated into generated C# code without specifying culture. On locales like fi_FI, negative numbers use the Unicode minus sign (U+2212) instead of the ASCII hyphen-minus, making the generated code invalid.
Fix:
Wrapped the entire source generator execution (both parsing and emitting) with code that sets
CultureInfo.CurrentCulturetoInvariantCultureand restores it in a finally block. This comprehensive approach:Files Modified:
using System.Globalization;since culture is now managed in calling codeTests:
NegativeJsonPropertyOrderGeneratesValidCodethat tests negative JsonPropertyOrder with fi_FI culture using ThreadCultureChange helperFixes #121277
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.